Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Offer 時は DataChannel を作る #243

Merged
merged 2 commits into from
Mar 5, 2022
Merged

Conversation

tnoho
Copy link
Contributor

@tnoho tnoho commented Feb 19, 2022

#237 対応で CreateOffer 時に CreateDataChannel します。

チャンネル名は固定してありますが、そもそも Serial と DataChannel をつなぐ部分がチャンネル名で接続先の Serial を変えるような実装をしていないため支障ありません。

CreateOffer 時に DataManager があれば CreateDataChannel っていうのはちょっと雑な実装な気もしつつ、この位置で行えば変更が少なくて済むのと、 Ayame しか CreateOffer しませんし ReOffer することもないのでコマンドラインツールとしては許容範囲だと判断しました。

いつもは PR 作成前に動作検証するのですが、これはちょっと検証が大変なのでやっていません。時間があったらやってみます。

@tnoho tnoho self-assigned this Feb 19, 2022
@voluntas
Copy link
Member

@tnoho 説明に書いてくれている通り、そもそも label 名で何か挙動を変えているわけではないだろうから問題ないという認識。むしろこれ serial みたいな label を使うだと ayame 連携で html 側いじらないといけないからいやです。

@@ -116,6 +116,18 @@ RTCConnection::~RTCConnection() {

void RTCConnection::CreateOffer(OnCreateSuccessFunc on_success,
OnCreateFailureFunc on_failure) {
// CreateOffer を行うのは Ayame だけのため、ここで Offer の場合には DataChannel を作ることとした
// Momo の性質上 ReOffer することは無いので問題ないと思われる
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@torikizi
Copy link
Contributor

torikizi commented Mar 5, 2022

M1 Mac と intel Mac での Serial 接続問題なく動作すること確認できました。

@voluntas voluntas merged commit bf6dee2 into develop Mar 5, 2022
@voluntas voluntas deleted the feature/add-create-datachannel branch March 5, 2022 06:09
nkkn1446 pushed a commit to nkkn1446/momo that referenced this pull request Oct 21, 2022
…tachannel

Offer 時は DataChannel を作る
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants